From f90d21f62f3c5c63ed878efa8ad34962cf4fa522 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 9 Sep 2017 13:52:03 -0700 Subject: [PATCH] Make `dep_targets` consistent throughout compilation Previously it depended on dynamic state that was calculated throughout a compilation which ended up causing different fingerprints showing up in a few locations, so this makes the invocation deterministic throughout `cargo_rustc`. --- src/cargo/ops/cargo_rustc/context.rs | 33 +++++++++++------------ src/cargo/ops/cargo_rustc/custom_build.rs | 32 ++++++++++++---------- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index ce3bba0a8..56bbe9470 100755 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -40,6 +40,7 @@ pub struct Context<'a, 'cfg: 'a> { pub compilation: Compilation<'cfg>, pub packages: &'a PackageSet<'cfg>, pub build_state: Arc, + pub build_script_overridden: HashSet<(PackageId, Kind)>, pub build_explicit_deps: HashMap, BuildDeps>, pub fingerprints: HashMap, Arc>, pub compiled: HashSet>, @@ -156,6 +157,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { used_in_plugin: HashSet::new(), incremental_enabled: incremental_enabled, jobserver: jobserver, + build_script_overridden: HashSet::new(), // TODO: Pre-Calculate these with a topo-sort, rather than lazy-calculating target_filenames: HashMap::new(), @@ -499,7 +501,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { self.resolve.features_sorted(unit.pkg.package_id()).hash(&mut hasher); // Mix in the target-metadata of all the dependencies of this target - if let Ok(deps) = self.used_deps(unit) { + if let Ok(deps) = self.dep_targets(unit) { let mut deps_metadata = deps.into_iter().map(|dep_unit| { self.target_metadata(&dep_unit) }).collect::>(); @@ -695,10 +697,18 @@ impl<'a, 'cfg> Context<'a, 'cfg> { Ok(Arc::new(ret)) } - fn used_deps(&self, unit: &Unit<'a>) -> CargoResult>> { + /// For a package, return all targets which are registered as dependencies + /// for that package. + pub fn dep_targets(&self, unit: &Unit<'a>) -> CargoResult>> { + if unit.profile.run_custom_build { + return self.dep_run_custom_build(unit) + } else if unit.profile.doc && !unit.profile.test { + return self.doc_deps(unit); + } + let id = unit.pkg.package_id(); let deps = self.resolve.deps(id); - deps.filter(|dep| { + let mut ret = deps.filter(|dep| { unit.pkg.dependencies().iter().filter(|d| { d.name() == dep.name() && d.version_req().matches(dep.version()) }).any(|d| { @@ -747,20 +757,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { } Err(e) => Some(Err(e)) } - }).collect::>>() - } - - /// For a package, return all targets which are registered as dependencies - /// for that package. - pub fn dep_targets(&self, unit: &Unit<'a>) -> CargoResult>> { - if unit.profile.run_custom_build { - return self.dep_run_custom_build(unit) - } else if unit.profile.doc && !unit.profile.test { - return self.doc_deps(unit); - } - - let id = unit.pkg.package_id(); - let mut ret = self.used_deps(unit)?; + }).collect::>>()?; // If this target is a build script, then what we've collected so far is // all we need. If this isn't a build script, then it depends on the @@ -812,7 +809,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { // actually depend on anything, we've reached the end of the dependency // chain as we've got all the info we're gonna get. let key = (unit.pkg.package_id().clone(), unit.kind); - if self.build_state.outputs.lock().unwrap().contains_key(&key) { + if self.build_script_overridden.contains(&key) { return Ok(Vec::new()) } diff --git a/src/cargo/ops/cargo_rustc/custom_build.rs b/src/cargo/ops/cargo_rustc/custom_build.rs index 727f0d324..ec78ce4e4 100644 --- a/src/cargo/ops/cargo_rustc/custom_build.rs +++ b/src/cargo/ops/cargo_rustc/custom_build.rs @@ -77,7 +77,9 @@ pub fn prepare<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult<(Work, Work, Freshness)> { let _p = profile::start(format!("build script prepare: {}/{}", unit.pkg, unit.target.name())); - let overridden = cx.build_state.has_override(unit); + + let key = (unit.pkg.package_id().clone(), unit.kind); + let overridden = cx.build_script_overridden.contains(&key); let (work_dirty, work_fresh) = if overridden { (Work::noop(), Work::noop()) } else { @@ -314,18 +316,6 @@ impl BuildState { fn insert(&self, id: PackageId, kind: Kind, output: BuildOutput) { self.outputs.lock().unwrap().insert((id, kind), output); } - - fn has_override(&self, unit: &Unit) -> bool { - let key = unit.pkg.manifest().links().map(|l| (l.to_string(), unit.kind)); - match key.and_then(|k| self.overrides.get(&k)) { - Some(output) => { - self.insert(unit.pkg.package_id().clone(), unit.kind, - output.clone()); - true - } - None => false, - } - } } impl BuildOutput { @@ -483,7 +473,7 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, // Recursive function to build up the map we're constructing. This function // memoizes all of its return values as it goes along. fn build<'a, 'b, 'cfg>(out: &'a mut HashMap, BuildScripts>, - cx: &Context<'b, 'cfg>, + cx: &mut Context<'b, 'cfg>, unit: &Unit<'b>) -> CargoResult<&'a BuildScripts> { // Do a quick pre-flight check to see if we've already calculated the @@ -492,6 +482,20 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, return Ok(&out[unit]) } + { + let key = unit.pkg.manifest().links().map(|l| (l.to_string(), unit.kind)); + let build_state = &cx.build_state; + if let Some(output) = key.and_then(|k| build_state.overrides.get(&k)) { + let key = (unit.pkg.package_id().clone(), unit.kind); + cx.build_script_overridden.insert(key.clone()); + build_state + .outputs + .lock() + .unwrap() + .insert(key, output.clone()); + } + } + let mut ret = BuildScripts::default(); if !unit.target.is_custom_build() && unit.pkg.has_custom_build() { -- 2.30.2